Ap, LinearSVM, OGD as estimators #849

sfilipi · 2018-09-06T18:44:34Z

Converting AveragePerceptron, LinearSVM and ODG trainers to estimators.

sfilipi · 2018-09-06T19:37:19Z

src/Microsoft.ML.StandardLearners/Standard/Online/AveragedLinear.cs

@@ -52,9 +53,10 @@ public abstract class AveragedLinearArguments : OnlineLinearArguments
        public Float AveragedTolerance = (Float)1e-2;
    }

-    public abstract class AveragedLinearTrainer<TArguments, TPredictor> : OnlineLinearTrainer<TArguments, TPredictor>
+    public abstract class AveragedLinearTrainer<TArguments, TTransformer, TModel> : OnlineLinearTrainer<TArguments, TTransformer, TModel>


TArguments [](start = 48, length = 10)

I will remove this on the next iteration, together with updating the tests. #Closed

Updated test.

TomFinley · 2018-09-06T23:04:50Z

src/Microsoft.ML.Core/Data/IEstimator.cs

@@ -237,7 +237,7 @@ public interface IEstimator<out TTransformer>
        /// <summary>
        /// Train and return a transformer.
        /// </summary>
-        TTransformer Fit(IDataView input);
+        TTransformer Fit(IDataView input, IDataView validationData = null, IPredictor initialPredictor = null);


IDataView validationData = null, IPredictor initialPredictor = null) [](start = 41, length = 69)

I am not certain I welcome this move. It seems like it is repeating the mistake of IIncrementalValidatingTrainer, which would be bad enough if it was just for trainers alone but it seems to be for absolutely every estimator. #Resolved

I agree

In reply to: 215803874 [](ancestors = 215803874)

TomFinley

Hi @sfilipi ! To change such a central interface as IEstimator with a signature like this is not something we should welcome. The argument I made in #509, which I believe is absolutely correct, is that what is fed to a trainer is not something fixed now and forever. We already have beliefs about more things that ought to be given to these things. This is by itself bad enough, but to putting them in an interface specifically is an especially hazardous venture since changes to interfaces between versions is somewhat disastrous. That we're encouraging this on IEstimator which is, frankly, arguably the most important interface in the entire ecosystem, makes me very nervous. So for this reason I am very, very alarmed.

sfilipi · 2018-09-06T23:38:20Z

test/Microsoft.ML.Tests/Scenarios/Api/Estimators/TrainWithInitialPredictor.cs

-                var secondTrainer = new MyAveragedPerceptron(env, new AveragedPerceptronTrainer.Arguments(), "Features", "Label");
-                var finalModel = secondTrainer.Train(trainData, firstModel.Model);
+                var secondTrainer = new AveragedPerceptronTrainer(env, new AveragedPerceptronTrainer.Arguments());
+                var finalModel = secondTrainer.Fit(trainData, initialPredictor: firstModel.Model);


Fit [](start = 47, length = 3)

@[email protected] if Fit() should not take a validation set, and an initial model, for this example/case here, would we still call Train()?

// Train the second predictor on the same data.
var secondTrainer = new AveragedPerceptronTrainer(env, new AveragedPerceptronTrainer.Arguments());

var trainRoles = new RoleMappedData(trainData, label: "Label", feature: "Features"); var finalModel = secondTrainer.Train(new TrainContext(trainRoles, initialPredictor: firstModel.Model)); #Resolved

In Pigsty land, I'm not sure how this can be accommodated (or rather should it?)

In Dynamic land, yes, we still call Train, just not with the TrainContext and RoleMappedData

In reply to: 215809082 [](ancestors = 215809082)

In Pigsty land, I'm not sure how this can be accommodated (or rather should it?)

Almost certainly, but unless I'm mistaken I don't think changes to IEstimator, or any of these core interfaces, will be necessary, or indeed even slightly helpful. Indeed, I might be missing something, but I'm a little confused about where we imagine the source of the problem is? We've posited the existence of some hypothetical ITransformer Train(IDataView trainData, ...) method, so why do we suppose a Transformer<TIn, TOut> Train<TIn, TOut>(DataView<TIn> trainData, ...) is impossible? Perhaps there's some subtlety I've missed.

In reply to: 215821268 [](ancestors = 215821268,215809082)

I'm not certain whether that's in scope for this PR. Maybe it is, I just don't know.

In reply to: 215840658 [](ancestors = 215840658,215821268,215809082)

sfilipi · 2018-09-07T00:16:04Z

src/Microsoft.ML.StandardLearners/Standard/Online/AveragedPerceptron.cs

    {
        public const string LoadNameValue = "AveragedPerceptron";
        internal const string UserNameValue = "Averaged Perceptron";
        internal const string ShortName = "ap";
        internal const string Summary = "Averaged Perceptron Binary Classifier.";

+        internal new readonly Arguments Args;


internal [](start = 8, length = 8)

this should truly be private, but our analyzer wants the private properties to be lowercased. This i should change or add a separate rule for 'private new'. Thoughts? #Closed

why not just private readonly Arguments _args ?

In reply to: 215814336 [](ancestors = 215814336)

Zruty0 · 2018-09-07T01:11:11Z

test/Microsoft.ML.Tests/Scenarios/Api/Estimators/TrainWithInitialPredictor.cs

-                var finalModel = secondTrainer.Train(trainData, firstModel.Model);
+                var secondTrainer = new AveragedPerceptronTrainer(env, new AveragedPerceptronTrainer.Arguments());
+
+                var trainRoles = new RoleMappedData(trainData, label: "Label", feature: "Features");


RoleMappedData [](start = 37, length = 14)

wait, wait, RoleMappedData should not survive :) #Pending

I mean, I'm fine with keeping them internally for now, but Train call should take only IDataView for train data

In reply to: 215820969 [](ancestors = 215820969)

What is the preferred way of passing the initialPredictor for online learners if neither Train or Fit should take them in as arguments?

In reply to: 215821958 [](ancestors = 215821958,215820969)

@Zruty0 i added a second TrainContext that contains IDataView for the Training, Validation data, and another Train Method that takes this second TrainContext but that proliferates everywhere in an ugly way.

I think we should keep the RoleMappedData instead of this spawn, and once it goes away, TrainContext will get cleaned up and so will this example. Objections?

In reply to: 215848474 [](ancestors = 215848474,215821958,215820969)

that's ok, let's create an issue for that though.

In reply to: 216079699 [](ancestors = 216079699,215848474,215821958,215820969)

Zruty0 · 2018-09-07T01:17:38Z

src/Microsoft.ML.StandardLearners/Standard/Online/OnlineGradientDescent.cs

@@ -25,10 +25,11 @@

 namespace Microsoft.ML.Runtime.Learners
 {
+    using Microsoft.ML.Core.Data;


using [](start = 4, length = 5)

consolidate #Closed

Zruty0 · 2018-09-07T01:17:44Z

src/Microsoft.ML.StandardLearners/Standard/Online/OnlineGradientDescent.cs

@@ -25,10 +25,11 @@

 namespace Microsoft.ML.Runtime.Learners
 {
+    using Microsoft.ML.Core.Data;
    using TPredictor = LinearRegressionPredictor;


TPredictor [](start = 10, length = 10)

let's remove this #Closed

Zruty0 · 2018-09-07T01:18:01Z

using Float = System.Single;

Remove this please #Closed

Refers to: src/Microsoft.ML.StandardLearners/Standard/Online/OnlineLinear.cs:19 in 0c176aa. [](commit_id = 0c176aa, deletion_comment = False)

Zruty0

🕐

Zruty0

TomFinley

Thanks @sfilipi .

TomFinley · 2018-09-08T21:01:27Z

src/Microsoft.ML.StandardLearners/Standard/Online/OnlineLinear.cs

@@ -2,8 +2,11 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using Float = System.Single;
+


I think @Zruty0 meant remove it altogether, but that's all right, we can do a sweep for this later.

sfilipi added 2 commits September 6, 2018 11:39

Converting AveragePerceptron, OGD and Linear SVM to estimators.

96fd88e

Added Propability to the output columns of binary

d47014e

sfilipi added the API Issues pertaining the friendly API label Sep 6, 2018

sfilipi added this to the 0918 milestone Sep 6, 2018

sfilipi requested review from Ivanidzo4ka, TomFinley and Zruty0 September 6, 2018 18:44

sfilipi commented Sep 6, 2018

View reviewed changes

fixing MakeLabel for OGD

23da0eb

Zruty0 mentioned this pull request Sep 6, 2018

New API for ML.NET #754

Closed

Fit should take an optional InitialPredictor for the OnlineTrainers.

101c2e8

Updated test.

Zruty0 assigned sfilipi Sep 6, 2018

Removing the arguments from the generics definition

58dbbac

sfilipi changed the title ~~WIP: Ap estimator~~ Ap, linearSVM, OGD as estimators Sep 6, 2018

sfilipi changed the title ~~Ap, linearSVM, OGD as estimators~~ Ap, LinearSVM, OGD as estimators Sep 6, 2018

TomFinley reviewed Sep 6, 2018

View reviewed changes

TomFinley suggested changes Sep 6, 2018

View reviewed changes

sfilipi commented Sep 6, 2018

View reviewed changes

Reverting the signature change on Fit()

0c176aa

sfilipi commented Sep 7, 2018

View reviewed changes

Zruty0 reviewed Sep 7, 2018

View reviewed changes

Zruty0 suggested changes Sep 7, 2018

View reviewed changes

sfilipi added 2 commits September 6, 2018 22:26

addressing comments

6a01926

ordering usings

40fcd33

Zruty0 approved these changes Sep 7, 2018

View reviewed changes

TomFinley approved these changes Sep 8, 2018

View reviewed changes

TomFinley reviewed Sep 8, 2018

View reviewed changes

TomFinley merged commit df499aa into dotnet:master Sep 8, 2018

sfilipi deleted the apEstimator branch October 9, 2018 22:24

ghost locked as resolved and limited conversation to collaborators Mar 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ap, LinearSVM, OGD as estimators #849

Ap, LinearSVM, OGD as estimators #849

sfilipi commented Sep 6, 2018

sfilipi Sep 6, 2018 •

edited

Loading

TomFinley Sep 6, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 7, 2018

TomFinley left a comment •

edited

Loading

sfilipi Sep 6, 2018 •

edited

Loading

Zruty0 Sep 7, 2018

TomFinley Sep 7, 2018

TomFinley Sep 7, 2018

sfilipi Sep 7, 2018 •

edited by Zruty0

Loading

Zruty0 Sep 7, 2018

Zruty0 Sep 7, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 7, 2018

sfilipi Sep 7, 2018 •

edited

Loading

sfilipi Sep 7, 2018 •

edited

Loading

Zruty0 Sep 7, 2018

Zruty0 Sep 7, 2018 •

edited

Loading

Zruty0 Sep 7, 2018 •

edited

Loading

Zruty0 commented Sep 7, 2018 •

edited

Loading

Zruty0 left a comment

Zruty0 left a comment

TomFinley left a comment

TomFinley Sep 8, 2018

Ap, LinearSVM, OGD as estimators #849

Ap, LinearSVM, OGD as estimators #849

Conversation

sfilipi commented Sep 6, 2018

sfilipi Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Sep 6, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley left a comment • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Sep 7, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 7, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 commented Sep 7, 2018 • edited Loading

Zruty0 left a comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Sep 6, 2018 •

edited

Loading

TomFinley Sep 6, 2018 •

edited by sfilipi

Loading

TomFinley left a comment •

edited

Loading

sfilipi Sep 6, 2018 •

edited

Loading

sfilipi Sep 7, 2018 •

edited by Zruty0

Loading

Zruty0 Sep 7, 2018 •

edited by sfilipi

Loading

sfilipi Sep 7, 2018 •

edited

Loading

sfilipi Sep 7, 2018 •

edited

Loading

Zruty0 Sep 7, 2018 •

edited

Loading

Zruty0 Sep 7, 2018 •

edited

Loading

Zruty0 commented Sep 7, 2018 •

edited

Loading